-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AMIFamily Defaulting and Validation #1296
Conversation
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 4c70160 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/62030d58a54f7c0008a2e53c |
fd52b52
to
cf3f1a2
Compare
OperatingSystemBottleRocket = "Bottlerocket" | ||
OperatingSystemEKSOptimized = "EKSOptimized" | ||
AMIFamilyBottlerocket = "Bottlerocket" | ||
AMIFamilyEKSOptimized = "EKSOptimized" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to make this EKSOptimizedLinux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just support Operating system separately and keep AMIFamily the actual family? So if AMIFamily was EKSOptimized but the pod requested Windows, we'd just give them windows EKS Optimized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes a certain amount of parity between EKSOptimized. In general, I agree with you. IIRC, this was a request from @suket22.
de22561
to
f1cb013
Compare
} | ||
} else { | ||
userData, err = p.getEKSOptimizedUserData(ctx, constraints, instanceTypes, additionalLabels) | ||
userData := p.getEKSOptimizedUserData(ctx, constraints, instanceTypes, additionalLabels, caBundle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally minor, but if you factored this out into a function, you could use the same return pattern used in the ami logic and keep this core Get() function as simple as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this?
1. Issue, if available:
#923
2. Description of changes:
Some AMIFamily updates to support:
3. How was this change tested?
Tested in an EKS cluster using both Bottlerocket and EKS Optimized AMI
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.